Skip to content

feat(signals): warn when reactive method runs with source injector#4742

Merged
markostanimirovic merged 13 commits intongrx:mainfrom
rainerhahnekamp:feat/signals/warning-outside-reactive-context
Mar 29, 2025
Merged

feat(signals): warn when reactive method runs with source injector#4742
markostanimirovic merged 13 commits intongrx:mainfrom
rainerhahnekamp:feat/signals/warning-outside-reactive-context

Conversation

@rainerhahnekamp
Copy link
Copy Markdown
Contributor

@rainerhahnekamp rainerhahnekamp commented Mar 28, 2025

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

It gives a warning when rxMethod or signalMethod's functions are called outside an injection context and therefore bare the risk of memory leaks.

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

Closes #4726

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@netlify
Copy link
Copy Markdown

netlify bot commented Mar 28, 2025

Deploy Preview for ngrx-io canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit afb3bfb
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/67e80b988eea7f0008db2b70

@rainerhahnekamp rainerhahnekamp changed the title Feat/signals/warning outside reactive context feat(signals): warn when reactive method runs with source injector Mar 28, 2025
@rainerhahnekamp rainerhahnekamp force-pushed the feat/signals/warning-outside-reactive-context branch 2 times, most recently from 7713962 to 3f8835a Compare March 28, 2025 14:48
@rainerhahnekamp rainerhahnekamp marked this pull request as ready for review March 28, 2025 14:52
@rainerhahnekamp rainerhahnekamp requested a review from Copilot March 28, 2025 14:52
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a warning when reactive methods (signalMethod and rxMethod) are invoked outside an injection context to help prevent memory leaks. The changes include updates to documentation, modifications to the core implementation to display a warning, and new tests to verify the warning behavior.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
projects/ngrx.io/content/guide/signals/signal-method.md Updated documentation alert message for signalMethod
projects/ngrx.io/content/guide/signals/rxjs-integration.md Revised documentation text for reactive method warnings
modules/signals/src/signal-method.ts Added warning on missing injector, updated getCallerInjector return
modules/signals/spec/signal-method.spec.ts Added tests for warning behavior in signalMethod
modules/signals/rxjs-interop/src/rx-method.ts Added warning on missing injector, updated getCallerInjector return
modules/signals/rxjs-interop/spec/rx-method.spec.ts Added tests for warning behavior in rxMethod
Comments suppressed due to low confidence (4)

projects/ngrx.io/content/guide/signals/signal-method.md:94

  • [nitpick] Consider clarifying the alert message by referencing that an explicit injector can be provided via the config parameter, to align with the implementation details.
<div class="alert is-important">

projects/ngrx.io/content/guide/signals/rxjs-integration.md:243

  • [nitpick] For clarity, consider explicitly naming the reactive method (e.g., rxMethod) in the warning description so that the documentation clearly communicates which methods are affected.
-If the injector is not provided when calling the reactive method outside of current injection context, the cleanup occurs when reactive method is destroyed.

modules/signals/src/signal-method.ts:36

  • [nitpick] The multiline warning message in the console.warn call contains extra indentation that may lead to unintended whitespace in the output; consider formatting the string to remove excessive leading whitespace.
if (config?.injector === undefined && callerInjector === undefined) {

modules/signals/rxjs-interop/src/rx-method.ts:45

  • [nitpick] Similar to the signalMethod implementation, ensure that the multiline warning message in the subsequent console.warn call is formatted to avoid unintended leading whitespace in the console output.
const callerInjector = getCallerInjector();

Copy link
Copy Markdown
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I've added a comment to make it clear that the message comes from us.
Do we also want to disable this in production?

I was also thinking if this should be something to be able to disable, but I can't think of any good use case for doing so.

@rainerhahnekamp
Copy link
Copy Markdown
Contributor Author

Do we also want to disable this in production?

I would keep it turned on. Generally speaking, I think it might be better to remove the possibility of using the source injector in the long run. It seems to me to be too risky...

@rainerhahnekamp rainerhahnekamp force-pushed the feat/signals/warning-outside-reactive-context branch from 447c2d4 to cad51a4 Compare March 29, 2025 12:43
rainerhahnekamp and others added 11 commits March 29, 2025 13:44
…n context

Emit a warning when a reactive method created via `rxMethod` or `signalMethod`
is invoked outside of an injection context without a manually provided injector.

This helps detect potential memory leaks caused by mismatched injection
contexts — for instance, when the method is defined in a service
provided in root but invoked in a component’s `ngOnInit`.
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
@rainerhahnekamp rainerhahnekamp force-pushed the feat/signals/warning-outside-reactive-context branch from cad51a4 to fa72689 Compare March 29, 2025 12:44
Copy link
Copy Markdown
Member

@markostanimirovic markostanimirovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👌

@markostanimirovic markostanimirovic merged commit 84a537c into ngrx:main Mar 29, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

@ngrx/signals: Display warning when reactive method is executed outside of caller's injection context

4 participants